-
Notifications
You must be signed in to change notification settings - Fork 748
[EXIR] Register _clone_dim_order op and map aten.clone #13735
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/13735
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 New FailuresAs of commit 30a1d13 with merge base 32e82bc ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@pytorchbot label "release notes: none" |
|
@Gasoonjia I could use your input on the Qualcomm backend changes: After replacing Should I set the clone op removal condition to the default value (True) instead of checking if dim order is non-contiguous. I also didn't create a node visitor or add an operator test in test_qnn_delegate since the op wouldn't be delegated and would fail the delegated partitioner count test. |
|
Hi @keyprocedure , thanks for your great work and ci has been triggered! Yes we can treat the dim order variant copy as non-delegated, just as what we are doing for aten::copy. cc. @digantdesai |
|
Happy to help! |
|
@Gasoonjia CI failures are unrelated. If everything looks good to you, I can commit the |
|
@keyprocedure lgtm. Please solve the conflict and then I will stamp it! |
|
@Gasoonjia the merge conflict is resolved. I noticed that coreml's torch_ops.py has updated the way it registers |
|
To add the ciflow label This helps ensure we don't trigger CI on this PR until it is actually authorized to do so. Please ping one of the reviewers if you do not have access to approve and run workflows. |
|
To add the ciflow label This helps ensure we don't trigger CI on this PR until it is actually authorized to do so. Please ping one of the reviewers if you do not have access to approve and run workflows. |
|
@keyprocedure i think it is fine for us right now, and we can work on coreml side later. @metascroy do you have any concerns on this? |
| to(context, node) | ||
|
|
||
|
|
||
| @register_torch_op( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Yes, the changes in torch_ops.py should be sufficient. The purpose of torch_ops.py is to register ops that are not yet in coremltools. With that said, we generally prefer that you put up a PR in coremltools and link that PR in a comment above the op you're creating in toch_ops.py. You can see this for most other ops in the file, e.g.,
|
Gasoonjia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @metascroy 's comment! Let me stamp this PR for now and we can add coreml PR later.
### Summary This is PR 3 of 3 implementing a dim order aware clone op. This PR updates the clone removal pass to retain layout changing `aten.clone` and `_clone_dim_order` ops and remove no-op clones, ensuring layout/dim order is preserved through export. Related PRs: - PR 1: [#12974](#12974) - Add `_clone_dim_order` portable kernel - PR 2: [#13735](#13735) - Register `_clone_dim_order` op and map `aten.clone` Fixes #12645 ### Test plan Added tests to verify: - Clones that change layout are preserved. - Clones with unchanged layout (identity ops) are removed. All tests pass via: python -m unittest exir.tests.test_memory_format_ops_pass python -m unittest backends.transforms.test.test_remove_clone_ops --------- Co-authored-by: Gasoonjia <[email protected]>
Summary
This is PR 2 of 3 implementing a dim order aware clone op.
This PR registers the new
_clone_dim_orderop and mapsaten.clonetodim_order_ops._clone_dim_orderin EXIR during export to preserve memory layout changes (contiguous/channels_last). It also updates the Core ML, ARM, and Qualcomm backends to handle the new clone op.Related PRs:
_clone_dim_orderportable kernelFixes #12645
Test plan
All tests pass via:
python -m unittest exir.tests.test_memory_format_ops_passpython -m unittest backends.apple.coreml.test.test_torch_opspytest backends/arm/test/ops/test_clone.pypytest backends/arm/test/passes/test_remove_clone_pass.py